Update SEPA Requests to Encodable
#1173
Conversation
| import Foundation | ||
|
|
||
| /// The POST body for `v1/payment_methods/sepa_debit_accounts` | ||
| struct SEPADebitAccountsRequest: Encodable { |
There was a problem hiding this comment.
❓ - do we want to create a /Models directory within each Sources/<PaymentMethod> directory? Specifically to house these new classes specific to encoding (and maybe eventually to house ones specific to decoding if we go that route).
There was a problem hiding this comment.
Yeah, I think that would be helpful. I was thinking that as I was adding this. Should we do that as a follow up PR for all of the models we've created so far? I think it changes the source paths for the package managers (at least Cocoapods). Also down to do that for SEPA in this PR and just a follow up for the other 2. Thoughts?
There was a problem hiding this comment.
I like the idea of having those be 1 PR! But your call
There was a problem hiding this comment.
Cool - agree we can 🏈 on it in this PR to establish a pattern in a single PR moving forward
| case createMandateResult = "sepa_debit_account" | ||
| } | ||
|
|
||
| init(createMandateResult: CreateMandateResult) { |
There was a problem hiding this comment.
Nit - I would move the init to the bottom of this class.
Maybe these new Encodable structs is a good candidate for us to add to our style guide how we should organize these, MARKs, etc
There was a problem hiding this comment.
I had followed the pattern we established in BTApplePaymentTokensRequest where the nested struct is under the init. Though I do think having structs within structs could be making our preferred style more convoluted. Thoughts on removing the nesting?
There was a problem hiding this comment.
Yeh, looking back I think having all the struct definitions be near the property that uses it & pushing the init down is more readable vs having them intertwined. What do you think?
In terms of nested structs, I think they sometimes help readability to know when something is defined within the context of something else. For example with the ApplePaymentToken one, that name is pretty generic, but namespacing it the <Request>.ApplePaymentToken helps reduce name collisions.
Open to folks thoughts though!
There was a problem hiding this comment.
Yeh, looking back I think having all the struct definitions be near the property that uses it & pushing the init down is more readable vs having them intertwined. What do you think?
Yeah I am down if we feel like it makes things more readable! I pushed a commit here with the change: d479ab9
Also agree on style guiding whatever we decide (and finding a home for the style guide).
In terms of nested structs, I think they sometimes help readability to know when something is defined within the context of something else. For example with the ApplePaymentToken one, that name is pretty generic, but namespacing it the .ApplePaymentToken helps reduce name collisions.
That makes sense with naming collisions, I forgot about that aspect. Especially with something like card that could be a situation where a merchant has something similar by the same name.
There was a problem hiding this comment.
I added a backlog ticket for updating our style guide - DTBTSDK-3373
| let last4: String? | ||
| let merchantOrPartnerCustomerID: String? | ||
| let bankReferenceToken: String? | ||
| let mandateType: String? |
There was a problem hiding this comment.
nit(entirely optional): maybe we can add short in-line docs to describe each property within these structs
Summary of changes
postbodies used in the SEPA Direct Debit flow:SEPADebitAccountsRequestencodable typeSEPADebitRequestencodable typeSEPADirectDebitAPI_TestsChecklist
[ ] Added a changelog entryAuthors